Adding an interface to the system socket to help nydusd obtain config #504

DarkMountain-wyz · 2023-07-09T10:34:07Z

Issue

https://github.com/containerd/nydus-snapshotter/issues/388

Details

Add interface endpointGetDaemonConfig string="/api/v1/daemons/{id}/config" in system. go
Add handler func getDaemonConfig() to deliver config
Pass the system socket and URL to nydusd through command-line arguments

codecov · 2023-07-09T10:43:16Z

Codecov Report

Merging #504 (402b9e4) into main (dde2109) will increase coverage by 0.05%.
Report is 6 commits behind head on main.
The diff coverage is 38.55%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #504      +/-   ##
==========================================
+ Coverage   33.56%   33.61%   +0.05%     
==========================================
  Files          65       65              
  Lines        8176     8259      +83     
==========================================
+ Hits         2744     2776      +32     
- Misses       5117     5167      +50     
- Partials      315      316       +1

Files	Coverage Δ
config/config.go	`32.37% <ø> (ø)`
config/global.go	`22.93% <0.00%> (-0.43%)`	⬇️
pkg/daemon/command/command.go	`54.86% <0.00%> (-2.02%)`	⬇️
config/daemonconfig/daemonconfig.go	`35.55% <86.48%> (+35.55%)`	⬆️
pkg/manager/daemon_adaptor.go	`0.00% <0.00%> (ø)`
pkg/system/system.go	`4.79% <0.00%> (-0.69%)`	⬇️

pkg/manager/daemon_adaptor.go

pkg/daemon/command/command.go

sctb512 · 2023-07-12T10:57:19Z

#388

The link for this issue is not valid.

pkg/daemon/command/command.go

pkg/manager/daemon_adaptor.go

sctb512 · 2023-07-12T11:16:18Z

Pass the system socket and URL to nydusd through command-line arguments

I am confused why we need this API? Could you tell me more about this?

sctb512 · 2023-07-12T11:25:12Z

Related to dragonflyoss/nydus#1361

DarkMountain-wyz · 2023-07-13T01:48:06Z

Pass the system socket and URL to nydusd through command-line arguments

I am confused why we need this API? Could you tell me more about this?

My initial plan was to provide an API in nydusd to pass config from SN to nydusd, but this would require opening the socket before creating the daemon. The thread creating the daemon would need to block until the API server thread received the passed config. Similarly, in SN, it is necessary to block after sending the config until the daemon is created. In the original logic, once the SN establishes a connection with the socket, it is considered that the daemon has been created. I think such changes will have a significant impact, bringing many uncontrollable impacts. Therefore, the current solution is to request config from nydusd to SN. This will not change the original Logical framework, but also add control items that are compatible with the old version of nydusd.

changweige · 2023-07-13T06:32:45Z

Is it possible only add the capability to nydus-snapshotter to share registry credentials to nydusd when nydusd requires it?
Other part of items should be persisted on disk like what we do now.

imeoer · 2023-07-31T02:40:57Z

I think in addition to considering that credentials cannot be persisted, we also need to consider how to promptly renew the registry token using the new registry user/pass when it changed (for example when a credential helper is used).

And how nydusd can use the updated config when serving new images after the user updates the nydusd config (for example tune the prefetch setting).

Therefore, I think it is feasible to have the snapshotter provide this API.

config/daemonconfig/daemonconfig.go

changweige · 2023-08-02T11:04:19Z

Sorry, need rebase

changweige

Thanks, but we may need more work to do

config/config.go

config/daemonconfig/daemonconfig.go

pkg/daemon/command/command.go

pkg/manager/daemon_adaptor.go

pkg/system/system.go

DarkMountain-wyz · 2023-08-06T13:50:17Z

The method has changed, and now snapshotter supports providing backend config to nydus instead of writing files.

config/config.go

misc/snapshotter/config.toml

config/daemonconfig/fuse.go

DarkMountain-wyz · 2023-08-18T01:22:19Z

Hello @sctb512 @changweige , I've submitted a Pull Request that introduces a new implementation approach. In this approach, the backend configuration's Auth information is not persisted. Instead, during the startup of nydusd, a URL is provided, and nydusd retrieves the Auth information by making a request to this URL. This design aims to enhance the security. Please review and share your thoughts on this approach. Thank you!

imeoer · 2023-08-22T06:23:11Z

@DarkMountain-wyz It seems we need to fix the CI.

DarkMountain-wyz · 2023-08-22T07:47:17Z

@DarkMountain-wyz It seems we need to fix the CI.

I will fix it.

changweige · 2023-08-31T03:14:49Z

need rebase

DarkMountain-wyz · 2023-08-31T11:10:29Z

need rebase

@changweige it has been rebased now.

imeoer · 2023-09-05T02:18:27Z

config/daemonconfig/fuse.go

+			c.Device.Backend.Config.Auth = backendAuth
+			c.Device.Backend.Config.RegistryToken = registryToken
+			return err
+		case BackendTypeOss:


The s3 backend should also be handled. :)

Perhaps a more elegant way would be to use struct tags and reflect, for example:

type BackendConfig struct { ... Auth string `json:"auth,omitempty" secret:"true"` RegistryToken string `json:"registry_token,omitempty" secret:"true"` AccessKeyID string `json:"access_key_id,omitempty" secret:"true"` AccessKeySecret string `json:"access_key_secret,omitempty" secret:"true"` ... }

The s3 backend should also be handled. :)

Sorry for this, the s3 backend only be added in https://github.com/dragonflyoss/image-service/blob/631db29759f4dceacaa7ff80b845a9d0107beef5/api/src/config.rs#L520, not in snapshshotter, maybe we don't need to handle it first.

imeoer · 2023-09-05T02:27:25Z

pkg/system/system.go

+					statusCode = http.StatusNotFound
+					return
+				}
+				jsonResponse(w, credentialConfig)


Maybe respond to the backendConfig directly here? In this way, when we add a new backend, we don't need to worry about handling miss here.

imeoer · 2023-09-25T03:31:23Z

config/daemonconfig/daemonconfig.go

 	default:
 		return errors.Errorf("unknown backend type %s", backendType)
 	}

 	return nil
 }
+
+func serializeWithSecretFilter(obj interface{}) map[string]interface{} {


Can we add a UT for this func?

imeoer · 2023-09-27T01:55:07Z

config/config.go

+	EnableStargz           bool        `toml:"enable_stargz"`
+	EnableReferrerDetect   bool        `toml:"enable_referrer_detect"`
+	TarfsConfig            TarfsConfig `toml:"tarfs"`
+	EnableCredentialSource bool        `toml:"enable_credential_source"`


EnableBackendSource bool toml:"enable_backend_source"

imeoer · 2023-09-27T01:55:23Z

config/daemonconfig/daemonconfig.go

@@ -124,11 +126,13 @@ type DeviceConfig struct {
 // We don't have to persist configuration file for fscache since its configuration
 // is passed through HTTP API.
 func DumpConfigFile(c interface{}, path string) error {
+	if config.IsCredentialSourceEnabled() {


IsBackendSourceEnabled

imeoer · 2023-09-27T01:56:49Z

pkg/manager/daemon_adaptor.go

@@ -25,6 +26,8 @@ import (
 	"github.com/containerd/nydus-snapshotter/pkg/prefetch"
 )

+const endpointGetCredential string = "/api/v1/daemons/%s/credential"


const endpointGetBackend string = "/api/v1/daemons/%s/backend"

imeoer · 2023-09-27T01:57:23Z

pkg/system/system.go

+	sc.router.HandleFunc(endpointGetCredential, sc.getCredential()).Methods(http.MethodGet)
+}
+
+func (sc *Controller) getCredential() func(w http.ResponseWriter, r *http.Request) {


getBackend

imeoer · 2023-09-27T01:57:45Z

config/daemonconfig/daemonconfig_test.go

+	require.Equal(t, newCfg.Device.Backend.Config.Proxy, cfg.Device.Backend.Config.Proxy)
+	require.Equal(t, newCfg.Device.Backend.Config.BlobURLScheme, cfg.Device.Backend.Config.BlobURLScheme)
+	require.NotEqual(t, newCfg.Device.Backend.Config.Auth, cfg.Device.Backend.Config.Auth)
+}


Add a blank line here.

…ce of auth information This change introduces a new startup option, enable_backend_source, which controls whether the authentication information is persisted or not. By default, enable_backend_source is set to false, which means that the authentication information will be persisted. Signed-off-by: Guijie Wang <[email protected]>

… with secret tag This commit introduces a new function, serializeWithSecretFilter, which filters fields with the 'secret' tag and prevents them from being persisted to local storage when the value is true. Signed-off-by: Guijie Wang <[email protected]>

This commit introduces a new interface that allows nydusd to request backend configurations from the snapshotter via a Unix domain socket. Transferring all backend configs is to cope with the addition of different backend in the future. Signed-off-by: Guijie Wang <[email protected]>

Signed-off-by: Guijie Wang <[email protected]>

imeoer

LGTM!

imeoer reviewed Jul 11, 2023

View reviewed changes

pkg/manager/daemon_adaptor.go Outdated Show resolved Hide resolved

imeoer reviewed Jul 11, 2023

View reviewed changes

pkg/daemon/command/command.go Outdated Show resolved Hide resolved

sctb512 reviewed Jul 12, 2023

View reviewed changes

pkg/daemon/command/command.go Outdated Show resolved Hide resolved

sctb512 reviewed Jul 12, 2023

View reviewed changes

pkg/daemon/command/command.go Outdated Show resolved Hide resolved

pkg/manager/daemon_adaptor.go Outdated Show resolved Hide resolved

imeoer approved these changes Jul 31, 2023

View reviewed changes

imeoer mentioned this pull request Jul 31, 2023

Store auth in keyring and transfer it to nydusd via environment variable #512

Open

sctb512 reviewed Jul 31, 2023

View reviewed changes

config/daemonconfig/daemonconfig.go Outdated Show resolved Hide resolved

changweige requested changes Aug 2, 2023

View reviewed changes

DarkMountain-wyz closed this Aug 3, 2023

DarkMountain-wyz force-pushed the main branch from 635dacf to a6f4457 Compare August 3, 2023 07:04

DarkMountain-wyz reopened this Aug 6, 2023

imeoer reviewed Aug 10, 2023

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

DarkMountain-wyz requested review from sctb512 and changweige August 10, 2023 11:06

imeoer reviewed Aug 11, 2023

View reviewed changes

misc/snapshotter/config.toml Outdated Show resolved Hide resolved

config/daemonconfig/fuse.go Outdated Show resolved Hide resolved

DarkMountain-wyz closed this Aug 31, 2023

DarkMountain-wyz force-pushed the main branch from c88ee49 to 9a2575b Compare August 31, 2023 06:19

DarkMountain-wyz reopened this Aug 31, 2023

imeoer reviewed Sep 5, 2023

View reviewed changes

imeoer reviewed Sep 25, 2023

View reviewed changes

imeoer reviewed Sep 27, 2023

View reviewed changes

DarkMountain-wyz closed this Oct 1, 2023

DarkMountain-wyz force-pushed the main branch from 80396cf to dde2109 Compare October 1, 2023 01:49

Guijie Wang added 3 commits October 1, 2023 11:30

DarkMountain-wyz reopened this Oct 1, 2023

Guijie Wang added 5 commits October 31, 2023 16:15

Fixed CI errors

c64c219

Signed-off-by: Guijie Wang <[email protected]>

Fixed CI error

a078fd3

Signed-off-by: Guijie Wang <[email protected]>

Fixed CI error

2954505

Signed-off-by: Guijie Wang <[email protected]>

Fixed CI error

1561c95

Signed-off-by: Guijie Wang <[email protected]>

Fixed CI errors

402b9e4

Signed-off-by: Guijie Wang <[email protected]>

imeoer approved these changes Oct 31, 2023

View reviewed changes

changweige approved these changes Oct 31, 2023

View reviewed changes

changweige merged commit f894d55 into containerd:main Oct 31, 2023
16 checks passed

DarkMountain-wyz mentioned this pull request Nov 1, 2023

Implement nydusd and nydus-snapshotter to deliver config through the socket API dragonflyoss/nydus#1379

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding an interface to the system socket to help nydusd obtain config #504

Adding an interface to the system socket to help nydusd obtain config #504

DarkMountain-wyz commented Jul 9, 2023

codecov bot commented Jul 9, 2023 •

edited

Loading

sctb512 commented Jul 12, 2023 •

edited

Loading

sctb512 commented Jul 12, 2023

sctb512 commented Jul 12, 2023

DarkMountain-wyz commented Jul 13, 2023

changweige commented Jul 13, 2023

imeoer commented Jul 31, 2023 •

edited

Loading

changweige commented Aug 2, 2023

changweige left a comment

DarkMountain-wyz commented Aug 6, 2023 •

edited

Loading

DarkMountain-wyz commented Aug 18, 2023

imeoer commented Aug 22, 2023

DarkMountain-wyz commented Aug 22, 2023

changweige commented Aug 31, 2023

DarkMountain-wyz commented Aug 31, 2023

imeoer Sep 5, 2023

imeoer Sep 5, 2023

imeoer Sep 5, 2023

imeoer Sep 5, 2023

imeoer Sep 25, 2023

imeoer Sep 27, 2023

imeoer Sep 27, 2023

imeoer Sep 27, 2023

imeoer Sep 27, 2023

imeoer Sep 27, 2023

imeoer left a comment

Adding an interface to the system socket to help nydusd obtain config #504

Adding an interface to the system socket to help nydusd obtain config #504

Conversation

DarkMountain-wyz commented Jul 9, 2023

Issue

Details

codecov bot commented Jul 9, 2023 • edited Loading

Codecov Report

sctb512 commented Jul 12, 2023 • edited Loading

sctb512 commented Jul 12, 2023

sctb512 commented Jul 12, 2023

DarkMountain-wyz commented Jul 13, 2023

changweige commented Jul 13, 2023

imeoer commented Jul 31, 2023 • edited Loading

changweige commented Aug 2, 2023

changweige left a comment

Choose a reason for hiding this comment

DarkMountain-wyz commented Aug 6, 2023 • edited Loading

DarkMountain-wyz commented Aug 18, 2023

imeoer commented Aug 22, 2023

DarkMountain-wyz commented Aug 22, 2023

changweige commented Aug 31, 2023

DarkMountain-wyz commented Aug 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imeoer left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 9, 2023 •

edited

Loading

sctb512 commented Jul 12, 2023 •

edited

Loading

imeoer commented Jul 31, 2023 •

edited

Loading

DarkMountain-wyz commented Aug 6, 2023 •

edited

Loading